Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ENH] - Tech Audit #2: Accuracy checks for Hilbert Transform, Filters, and Spectra #204

Merged
merged 11 commits into from
Jul 13, 2020

Conversation

elybrand
Copy link
Contributor

@elybrand elybrand commented Jul 7, 2020

This PR adds accuracy checks (#194) for

  1. All hilbert.py related functions using a simple sinusoid.
  2. FIR filtering of a low-frequency sinusoid. Specifically, it checks that it is attenuated under a high pass filter, and is minimally attenuated under a lowpass filter.
  3. compute_spectrum with the medfilt and welch options. It uses a low frequency sinusoid to compare against where the power spectrum should be a dirac spike at frequency 1 and zero elsewhere.

@elybrand elybrand changed the title [ENH] - Time Frequency: Accuracy checks for Hilbert Transform [ENH] - Tech Audit #2: Accuracy checks for Hilbert Transform Jul 7, 2020
@elybrand elybrand changed the title [ENH] - Tech Audit #2: Accuracy checks for Hilbert Transform [ENH] - Tech Audit #2: Accuracy checks for Hilbert Transform, Filters Jul 8, 2020
Copy link
Member

@ryanhammonds ryanhammonds left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tests updates look good to me!

@TomDonoghue
Copy link
Member

TomDonoghue commented Jul 9, 2020

Okay, slightly unconventional review approach here. I thought I had just some small updates, so I started doing directly. Then I realized I had a bit more to suggest - which I thought I would add as comments, rather than digging in too deep with direct edits. So, in the end I did pushed an update commit, and I'm also leaving some review comments.

Please take a look at my direct commit, to sanity check, and get a sense of what I was going for. Basically, that commit is trying to consolidate on using general test settings and refactoring some code a bit.

@TomDonoghue
Copy link
Member

TomDonoghue commented Jul 9, 2020

@elybrand - thanks for the updates here! These look much better / more specific tests than we had before. I've made some comments / suggestions. A bunch of these things are more code mechanics, so if you prefer you can tag in @ryanhammonds to work on the code and organizational elements here, and we can coordinate this one as a team effort.

I also wasn't totally sure if this PR was done, or a work-in-progress (if you plan on adding more test updates). Note that if things are in progress, you can use the tag [WIP] in the title, so that everyone knows there's more coming.

@elybrand elybrand changed the title [ENH] - Tech Audit #2: Accuracy checks for Hilbert Transform, Filters [WIP] - Tech Audit #2: Accuracy checks for Hilbert Transform, Filters Jul 9, 2020
@elybrand elybrand changed the title [WIP] - Tech Audit #2: Accuracy checks for Hilbert Transform, Filters [WIP] - Tech Audit #2: Accuracy checks for Hilbert Transform, Filters, and Spectra Jul 9, 2020
@TomDonoghue
Copy link
Member

This looks great @elybrand! I've made some small comments for minor organizational things. Once you get a chance to check and update those, then I think this should be all good!

@elybrand elybrand changed the title [WIP] - Tech Audit #2: Accuracy checks for Hilbert Transform, Filters, and Spectra [ENH] - Tech Audit #2: Accuracy checks for Hilbert Transform, Filters, and Spectra Jul 13, 2020
@TomDonoghue
Copy link
Member

Looks good to me, merging in!

@TomDonoghue TomDonoghue merged commit 99c7a07 into neurodsp-tools:master Jul 13, 2020
@elybrand elybrand deleted the test_hilbert branch July 14, 2020 22:48
@ryanhammonds ryanhammonds added the 2.1 Updates to go into a 2.1.0 release label May 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.1 Updates to go into a 2.1.0 release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants